Skip to content

[4.4] Remove unused cassiopeia inline event handler#43586

Merged
MacJoom merged 4 commits intojoomla:4.4-devfrom
SniperSister:5.1-cassopeia-inline-event-handler
Jun 14, 2024
Merged

[4.4] Remove unused cassiopeia inline event handler#43586
MacJoom merged 4 commits intojoomla:4.4-devfrom
SniperSister:5.1-cassopeia-inline-event-handler

Conversation

@SniperSister
Copy link
Contributor

@SniperSister SniperSister commented May 31, 2024 •
edited
Loading

Summary of Changes

The current Cassiopeia version has an inline onload event handler for the fontscheme.current css file. The inline event handler produces a warning in strict CSP mode and is redundant, as the lazy-loading functionality (that was the purpose of the handler) is handled in the template.js and trigger by the rel="lazy-stylesheet" attribute.

Testing Instructions

  • Configure the "roboto (local)" font as font in cassiopeia, verify that it's loaded by the browser using the browser dev tools.
  • Configure a strict ("script-src: 'self' 'unsafe-eval'") report-only script CSP using the Joomla HTTP Header plugin:
Bildschirmfoto 2024-06-01 um 14 20 13

Actual result BEFORE applying this Pull Request

  • CSS file loaded
  • Error message about forbidden unsafe-inline JS calls, caused by an "onload" handler of the mentioned CSS file in the browser console shown
Bildschirmfoto 2024-06-01 um 14 21 00

Expected result AFTER applying this Pull Request

  • CSS file loaded
  • Error message about unsafe-inline calls about the onload handler is gone

Link to documentations

Please select:

  • Documentation link for docs.joomla.org:

  • [x ] No documentation changes for docs.joomla.org needed

  • Pull Request link for manual.joomla.org:

  • No documentation changes for manual.joomla.org needed

@SniperSister SniperSister requested a review from chmst as a code owner May 31, 2024 20:15
@brianteeman
Copy link
Contributor

brianteeman commented May 31, 2024

Please provide more test instructions and what is expected to be seen in the generated source

Maybe I am looking at this wrong but before this PR I see

<link href="/media/templates/site/cassiopeia/css/global/fonts-local_roboto.min.css?8eef51" rel="stylesheet" media="all" onload="this.media='all'">

With the PR applied I see

<link href="/media/templates/site/cassiopeia/css/global/fonts-local_roboto.min.css?447e02" rel="stylesheet" media="all" onload="this.media='all'">

@brianteeman
Copy link
Contributor

brianteeman commented May 31, 2024

Also does the same change need to be made in
templates\cassiopeia\component.php
templates\cassiopeia\error.php

dgrammatiko
dgrammatiko reviewed Jun 1, 2024
View reviewed changes
dgrammatiko
dgrammatiko suggested changes Jun 1, 2024
View reviewed changes
Copy link
Contributor

@dgrammatiko dgrammatiko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SniperSister please remove all the 'media' => 'print'

@SniperSister
Copy link
Contributor Author

SniperSister commented Jun 1, 2024

Lesson learned: don't commit while listening to JDay presentations ;)

Sorry guys, now all required changes (removal of inline handler, removal of unused media attribute) have been performed in all relevant files and locations.

@brianteeman
Copy link
Contributor

brianteeman commented Jun 1, 2024

As stated before please provide more detailed test instructions. How can a tester replicate the error etc

@SniperSister
Copy link
Contributor Author

SniperSister commented Jun 1, 2024

As stated before please provide more detailed test instructions. How can a tester replicate the error etc

Done!

@brianteeman
Copy link
Contributor

brianteeman commented Jun 1, 2024

I have tested this item ✅ successfully on f3bbd2d

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@richard67
Copy link
Member

richard67 commented Jun 2, 2024

@SniperSister Shouldn't it be fixed in 4.4-dev, too (or only there and later be merged up to 5.1-dev)? If it's a bug fix we should to it also in 4.4-dev, and if it's a new feature (what I don't think) it should to into 5.2-dev.

@dautrich
Copy link

dautrich commented Jun 2, 2024

I have tested this item ✅ successfully on f3bbd2d

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@richard67
Copy link
Member

richard67 commented Jun 2, 2024

RTC

This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/43586.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Jun 2, 2024
@SniperSister SniperSister changed the base branch from 5.1-dev to 4.4-dev June 3, 2024 06:21
@SniperSister SniperSister changed the base branch from 4.4-dev to 5.1-dev June 3, 2024 06:22
@SniperSister SniperSister force-pushed the 5.1-cassopeia-inline-event-handler branch from f3bbd2d to 82c15b5 Compare June 3, 2024 06:23
@joomla-cms-bot joomla-cms-bot added Language Change This is for Translators NPM Resource Changed This Pull Request can't be tested by Patchtester Unit/System Tests labels Jun 3, 2024
@SniperSister SniperSister changed the base branch from 5.1-dev to 4.4-dev June 3, 2024 06:23
@SniperSister
Copy link
Contributor Author

SniperSister commented Jun 3, 2024

Shouldn't it be fixed in 4.4-dev, too (or only there and later be merged up to 5.1-dev)?

@richard67 fair comment. I have rebased the PR as suggested!

@SniperSister SniperSister changed the title [5.1] Remove unused cassiopeia inline event handler [4.4] Remove unused cassiopeia inline event handler Jun 3, 2024
@joomla-cms-bot joomla-cms-bot added PR-4.4-dev and removed Language Change This is for Translators Unit/System Tests labels Jun 3, 2024
@SniperSister SniperSister added Language Change This is for Translators and removed PR-5.1-dev labels Jun 3, 2024
@joomla-cms-bot joomla-cms-bot removed the NPM Resource Changed This Pull Request can't be tested by Patchtester label Jun 3, 2024
@MacJoom MacJoom self-assigned this Jun 14, 2024
@joomla-cms-bot joomla-cms-bot removed the Language Change This is for Translators label Jun 14, 2024
@MacJoom MacJoom added this to the Joomla! 4.4.6 milestone Jun 14, 2024
@MacJoom MacJoom merged commit 67cd90e into joomla:4.4-dev Jun 14, 2024
@MacJoom
Copy link
Contributor

MacJoom commented Jun 14, 2024

Thank you!

@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Jun 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@chmst chmst Awaiting requested review from chmst

@laoneo laoneo Awaiting requested review from laoneo

@bembelimen bembelimen Awaiting requested review from bembelimen

@HLeithner HLeithner Awaiting requested review from HLeithner

@rdeutz rdeutz Awaiting requested review from rdeutz

@Hackwar Hackwar Awaiting requested review from Hackwar

1 more reviewer

@dgrammatiko dgrammatiko dgrammatiko requested changes

Reviewers whose approvals may not affect merge requirements

Assignees

@MacJoom MacJoom

Labels

PR-4.4-dev

Projects

None yet

Milestone

Joomla! 4.4.6

Development

Successfully merging this pull request may close these issues.

7 participants

@SniperSister @brianteeman @richard67 @dautrich @MacJoom @dgrammatiko @joomla-cms-bot